Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reworked parallel executor to not block #437

Merged
merged 7 commits into from
Sep 6, 2020
Merged

Reworked parallel executor to not block #437

merged 7 commits into from
Sep 6, 2020
+437 −210

Conversation

aclysma
Copy link
Contributor

@aclysma aclysma commented Sep 5, 2020

  • Added CountdownEvent - an awaitable counter that is ready after decremented N times
  • System tasks are generated and await a ready_event

@aclysma
Reworked parallel executor to not block
a099746 
 * Added CountdownEvent - an awaitable counter that is ready after decremented N times
 * System tasks are generated and await a ready_event
@aclysma
Copy link
Contributor Author

aclysma commented Sep 5, 2020

This is potentially a better fix for #405

@cart cart added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels Sep 5, 2020
cart
cart reviewed Sep 5, 2020
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just finished my first pass. I'm pretty happy with how this turned out. I especially like that it improved the legibility / grok-ability of the code. I think this is probably good to go, but I'll spend a bit more time running it / poking + prodding.

crates/bevy_ecs/src/schedule/parallel_executor.rs Show resolved Hide resolved
crates/bevy_ecs/src/schedule/parallel_executor.rs Outdated Show resolved Hide resolved
@aclysma aclysma mentioned this pull request Sep 5, 2020
Closed
@aclysma
Copy link
Contributor Author

aclysma commented Sep 5, 2020

Oh one comment about the ordering in CountdownTimer. Originally I left it all relaxed because there was always a SeqCst barrier being hit within Event that would apply to all memory. However adding the reset I'm not as certain as I was about the correctness, it's not necessarily great to be relying on an implementation detail within Event, and the ordering constraints should actually be free when there is a fence between them. (i.e. the if the fence is there, then the ordering constraints are satisfied and won't introduce any additional overhead.) (Not that it matters, these won't be under heavy contention.)

@cart
Copy link
Member

cart commented Sep 6, 2020

This looks good to me! Thanks for the hard work here!

@cart cart merged commit 8b35530 into bevyengine:master Sep 6, 2020
@GrantMoyer GrantMoyer mentioned this pull request Sep 22, 2020
Closed
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
@aclysma @mrk-its
Reworked parallel executor to not block (bevyengine#437)
b3cffea 
Reworked parallel executor to not block
This was referenced Mar 21, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@multun multun multun left review comments

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aclysma @cart @multun